🌱 OPRUN-4550: Replace generated mozilla_data.go with go:embed + runtime parsing#2634
🌱 OPRUN-4550: Replace generated mozilla_data.go with go:embed + runtime parsing#2634tmshort wants to merge 1 commit intooperator-framework:mainfrom
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR replaces the generated mozilla_data.go file with an embedded JSON approach, simplifying the TLS profile management. Instead of generating Go code from the Mozilla SSL/TLS Configuration Guidelines, the PR embeds the JSON data and parses it at runtime during package initialization. This eliminates the dependency on gojq and makes profile updates clearer through simple JSON diffs.
Changes:
- Embed
mozilla_data.jsonusinggo:embedand parse at init() time instead of generating Go code - Simplify
update-tls-profiles.shto just download the JSON file, removing all jq-based code generation - Add
TestNoSkippedCiphersto validate that all ciphers in the Mozilla data are supported by Go's crypto/tls - Remove
gojqdependency from the Makefile
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/shared/util/tlsprofiles/mozilla_data.go |
Rewritten to embed and parse mozilla_data.json at runtime, with helper functions to convert Mozilla cipher/curve names to Go TLS identifiers |
internal/shared/util/tlsprofiles/mozilla_data.json |
New embedded JSON file containing Mozilla TLS profile configurations for "modern" and "intermediate" profiles |
hack/tools/update-tls-profiles.sh |
Simplified from 131 lines to 12 lines - now just downloads JSON from Mozilla instead of generating Go code |
Makefile |
Removed gojq dependency from the update-tls-profiles target |
internal/shared/util/tlsprofiles/tlsprofiles_test.go |
Added TestNoSkippedCiphers to ensure all ciphers in the embedded data are supported by Go |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2634 +/- ##
==========================================
- Coverage 68.91% 68.88% -0.03%
==========================================
Files 139 140 +1
Lines 9863 9905 +42
==========================================
+ Hits 6797 6823 +26
- Misses 2559 2568 +9
- Partials 507 514 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| .PHONY: update-tls-profiles | ||
| update-tls-profiles: $(GOJQ) #EXHELP Update TLS profiles from the Mozilla wiki | ||
| env JQ=$(GOJQ) hack/tools/update-tls-profiles.sh |
There was a problem hiding this comment.
Seems to me here we drop the only usage of gojq, but we keep it still in bingo files (.bingo/gojq.mod, .bingo/Variables.mk, ...).
Should we drop it also from bingo?
Should be something like:
bingo-v0.9.0 get gojq@none
There was a problem hiding this comment.
Good catch, if not used anywhere else, we should remove it from bingo files as well.
There was a problem hiding this comment.
Done, and rebased, please re-review @pedjak
d3e0d27 to
26ad8ee
Compare
pedjak
left a comment
There was a problem hiding this comment.
Overall this is a great simplification — removes fragile shell code-gen and a dependency (gojq) in favor of go:embed + runtime parsing with existing helpers. A couple of inline suggestions below.
| } | ||
| cipherNums = append(cipherNums, id) | ||
| } | ||
|
|
There was a problem hiding this comment.
Unsupported ciphers are tracked in skippedCiphers and validated by TestNoSkippedCiphers — nice. But unsupported curves (where curveId() returns 0) are silently dropped here with no tracking or test.
If Mozilla adds a new curve that Go does not yet support, this would silently weaken the profile. Consider adding a skippedCurves slice with a corresponding test, mirroring the cipher handling:
id := curveId(c)
if id == 0 {
skipped = append(skipped, "curve:"+c)
continue
}(Or a separate skippedCurves list if you prefer to keep them distinct.)
There was a problem hiding this comment.
Yeah, I noticed that too... it may not even be a curve that isn't support, but simply not mapped in our own functions.
| func TestNoSkippedCiphers(t *testing.T) { | ||
| require.Empty(t, skippedCiphers, | ||
| "cipher(s) in mozilla_data.json are not supported by Go's crypto/tls and were omitted: %v", skippedCiphers) | ||
| } |
There was a problem hiding this comment.
Good test — catches newly-added ciphers that Go does not support yet.
Consider also adding a basic smoke test that asserts the parsed profile values match expectations (e.g. modernTLSProfile.minTLSVersion == tls.VersionTLS13, intermediate has ≥9 ciphers). This would catch upstream JSON schema changes that parse without error but produce wrong data. The existing tests exercise the profiles indirectly, so this is a nice-to-have rather than a blocker.
There was a problem hiding this comment.
minTLSVersion could change out from under us... i.e. intermediate could go to a newer version; so I'd prefer to avoid explicit value checks. Even the number of ciphers could drop as they are determined to be weak, so checking for a minimum number of ciphers is probably not what we want.
The missing cipher/curve option is a much more appropriate check.
Embeds mozilla_data.json at compile time and parses it in init() to populate modern and intermediate TLS profiles, removing the jq/gojq dependency. Tracks skipped ciphers and curves (names not supported by Go's crypto/tls) and asserts both are empty via TestNoSkippedCiphers and TestNoSkippedCurves. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Todd Short <tshort@redhat.com>
26ad8ee to
970a111
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pedjak, rashmigottipati The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Embed mozilla_data.json and parse it at init() time using the existing cipherSuiteId()/curveId() helpers. Unsupported ciphers are skipped and recorded in skippedCiphers, validated by TestNoSkippedCiphers. Simplify update-tls-profiles.sh to a curl download and drop the gojq dependency.
When the profile changes,
make verifywill show a diff of the profile. The build will still complete using the old data. The updated profile will need to be commited in order for it to be included in the build.Description
Reviewer Checklist